The definition of these 3 variables is unclear, please add a description. Also, it seems that only 2 out of 3 are needed no?

I will add comments and rename variables to make the code clearer.

I don't think 2 variables are enough:

• msm_size is the number of elements per msm sum
• batch_size is how many msms to compute in the batch
• nof_points can either be msm_size (if sharing points) or msm_size*batch_size (if not sharing points).

maybe I can replace nof_points by a boolean that tells if sharing points between batch elements or not. It would still be 3 variables and require modification of 'MSMConfig' which would break the API so I prefer avoiding it.

note that nof_scalars is always assumed to be msm_size * batch_size. Theoretically MSMs could share scalars but not points but I think it's not an interesting case and again I want to be fully compatible with current API.

I would leave three variables. By design it should be possible to have different points in the first and second MSMs and then re-use points from the first for the third and then points from the second for the fourth and so on...
In this case not_points = 2 * msm_size

making sure that this is the size of a single msm

yes it is. I will rename variables to make the code clearer.

this seems to be used a lot, maybe define total_nof_bms = nof_bms*batch_size

ok

why isn't this true for every batch size?

It is true for every batch size.
The thing is that for fields like BLS12-377 where the scalar field is 253b we end up with >253 bms (e.g. 254 bms for c=2 with 1 iteration of reduction from 127 to 254 bms. For other cases can be a little more than that) where the most significant ones are guaranteed to be zero.
The original code summed the bms based on bitsize so effectively summing only the non-zero bms. It is slightly more efficient more summing all bms (254 in the example I gave). I wrote the code this way for batch=1 since I did not want to end up with worse performance (although maybe negligible).

generally for batch>1 we must account for all bms so that the other batch elements are summing the right bms. If setting bm=253, the second batch element is summing wrong bms and all results end up wrong except for the first batch element.

Do you think I should always set bm based on the final number of windows? it is slightly less efficient but more drastic.

why not have each msm sum only the non zero buckets? (and make sure that the buckets are the correct ones - I'm pretty sure you can use bm=253 and correct the kernel to sum the correct buckets) let's go for best performance

Yes I think you are right. I will do that.

better use 32 threads min

ok

why is 16 the default for a single msm? it should also be the closest power of 2 to the optimal c

It was like that for batch=1 and so I did not want to end up with different behavior.

Should I use the closest power of 2 instead? closest from below? I mean does 12 goes to 8 to 16?

I don't remember.. use the value that gives better performance:)

I think the optimum is log(N) (meaning the number of ECADD is minimized) but implementation-wise power of 2 work better from what I read. In addition memory grows exponential in c since you have 2^c buckets per BM so practically c=16 is the largest power of two that is reasonable.

Having said that, the code says:
unsigned get_optimal_c(int bitsize) { return max((unsigned)ceil(log2(bitsize)) - 4, 1U); }

I don't understand why '-4'? It is quite odd since I then round it back to a power of two.

Anyway I will try to play with it a little and figure out what value are working better, but I think this is an optimization we can dedicate another PR and task for since I think it's a heuristic and not really part of this PR.

yes, for now just change 16 to get_optimal_c. (which will then be rounded to the closest power of 2)
the logn-4 is the value that minimizes the amount of ec additions for most sizes, but it's true that with the actual implementation there are other considerations too. Ideally we will have a dictionary.

We discussed in the daily that fine-tuning the value of c is probably out of the scope of this PR..
Imo it'll more appropriate to tabulate best values of c and optimal accumulation method (triangle vs. non-triangle) when we establish proper benchmarking and run out of options to significantly speed up the MSM (potentially altering choices that we hard-coded). I think for now it's ok to leave reasonable defaults and give users an option to edit them via config

I will leave it as is for now then, meaning the behavior remains as it is today.

Can't this procedure just hang for some values of c? Like, if c=b101, then c & (c - 1) doesn't add any extra bits so the process is never terminated. Also since the idea is rounding up I'm afraid any MSM of size bigger than 2^20 will end up with c=32 which takes way too much memory

it will never hang and it rounds down, not up:

• for powers of two exactly 1 bit is 1'b1 --> meaning 'c & (c-1)' is zero. for example for c=4'b0100, c-1 = 4'b0011.
• otherwise each iteration is masking one bit to 1'b0, making the number smaller and therefore it must end in at most 31 iterations.
• note that masking c with any number cannot result in a number larger than c, meaning it is rounding down.

in your example c=b101 therefore c-1=b100 --> c& (c-1) = b100. Note that the first non zero bit is masked to 1'b0 and next iteration would mask the next non zero bit.

oh sorry you're right

Just a style related note: google guide advices to basically use int everywhere. I know that we're still not following this rule fully, but we at least tried to do it in higher-level and public functions. Do you think it's a good idea to stick to it?

I know someone who is very experienced with CUDA and he uses uint32_t wherever he can:)

Well limbs obviously shouldn't be int. I'll just quote google guide to be more precise:

We use int very often, for integers we know are not going to be too big, e.g., loop counters. Use plain old int for such things.
...
You should not use the unsigned integer types such as uint32_t, unless there is a valid reason such as representing a bit pattern rather than a number, or you need defined overflow modulo 2^N. In particular, do not use unsigned types to say a number will never be negative. Instead, use assertions for this.

We discussed it in the daily meeting.
I personally prefer unsigned values (and actually uint32_t as Hadar mentioned since it's more portable and can avoid nasty bugs since sizeof(int) is not guaranteed to be 4 in all systems) when the value should not be negative because it tells the reader that the value should not be negative.

Two notes:
(1) google style suggests asserting the values are not negative instead of using unsigned int. I am not sure it is applicable to kernel code.
(2) when using std containers like std::vector, their size() method returns an unsigned value and therefore the compiler will warn about comparison if the loop counter is signed. Iterators are probably the better design pattern in those cases but this is not really the point here.

Anyway, unless you object, I will leave it as for now. Note that it's not exposed APIs but internal implementation.